Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create tags and captures_tags tables #16

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

SimonLab
Copy link
Member

@SimonLab SimonLab commented Apr 20, 2020

ref: #14

  • migration to create tags and captures_tags table in Postgres
  • Update the capture changeset to link a capture to tags using put_assoc
  • Preload tags when creating/updating/getting captures. This allow the api to return the captures with the linked tags

@SimonLab SimonLab self-assigned this Apr 20, 2020
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@d077e3a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #16   +/-   ##
=========================================
  Coverage          ?   81.67%           
=========================================
  Files             ?       22           
  Lines             ?      131           
  Branches          ?        0           
=========================================
  Hits              ?      107           
  Misses            ?       24           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d077e3a...d9e7c6c. Read the comment docs.

timestamps()
end

create unique_index(:tags, [:text])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This index makes sure the database doesn't contain tags with the same name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonLab which issue requirement or acceptance criteria is this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to follow the requirements from dwyl/app#245

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonLab thanks for clarifying. In which case it could be worth having that issue linked in the PR description and having it "in-progress" on the Kanban board (which makes it easier for the reviewer) 😉

|> put_assoc(:tags, parse_tags(attrs))
end

defp parse_tags(params) do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tags are passed as a string separated by a comma


defp insert_and_get_all(names) do
maps = Enum.map(names, &%{text: &1})
Repo.insert_all(Tag, maps, on_conflict: :nothing)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A conflict can appears when inserting tags, for example on a race condition. If mutiple tag are inserted at the same time with the same name the postgres index will report an error. The on_conflict catch this error and do nothing as at least one tag will be inserted with the correct name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonLab ideally the index should be based on the person_id and tag_id so that different people can have the same tag. But I think we need a dedicated issue for tag ownership. 💭

@SimonLab SimonLab mentioned this pull request Apr 20, 2020
3 tasks
@SimonLab
Copy link
Member Author

SimonLab commented Apr 23, 2020

I've added more test and now the coverage is above 80%
coverage

The remaining lines to cover are mostly linked to cases linked to the user authentication.
I'll look if there is a better way to tests them without using mock.
Now that the api allow a user to add tags to an item, I'm keen to see if it's working properly with github.com/dwyl/app-mvp-elm/

@SimonLab SimonLab assigned nelsonic and unassigned SimonLab Apr 23, 2020
@SimonLab SimonLab temporarily deployed to dwyl-app-api April 23, 2020 14:50 Inactive
@SimonLab SimonLab temporarily deployed to dwyl-app-api April 24, 2020 15:29 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants